[DRAFT] Implement Metadata record support #58
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements initial metadata record support by adding a Metadata data type and integrating it into the serialization pipeline. This is a draft implementation focused on getting the basic structure in place.
- Adds a new
Metadatarecord type with serialization/deserialization capabilities - Updates serialization functions to accept metadata streams alongside data streams
- Implements metadata roundtrip testing to verify the functionality
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scls-format.cabal | Adds the new Metadata module to the library exports |
| Cardano/SCLS/Internal/Record/Metadata.hs | New module defining Metadata record structure and serialization logic |
| Cardano/SCLS/Internal/Reader.hs | Adds withRecordData function for streaming records by type |
| Cardano/SCLS/Internal/Serializer/Reference/Impl.hs | Updates serialize function signature to accept metadata stream |
| Cardano/SCLS/Internal/Serializer/Reference/Dump.hs | Integrates metadata writing into the dump process |
| Cardano/SCLS/Internal/Serializer/External/Impl.hs | Updates external serializer to handle metadata streams |
| test/Roundtrip.hs | Adds metadata roundtrip testing and updates function signatures |
| test/MultiNamespace.hs | Updates test to accommodate new serialize function signature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| annotate | ||
| "Metadata stream roundtrip successful" | ||
| $ (decoded_metadata) | ||
| `shouldBe` ([mkMetadata bytes 1024 | bytes <- encoded_data]) -- TODO: should be sorted |
There was a problem hiding this comment.
The comparison uses encoded_data instead of sorted_encoded_data, which is inconsistent with the data stream test above. This could cause test failures when the original data order differs from sorted order.
| `shouldBe` ([mkMetadata bytes 1024 | bytes <- encoded_data]) -- TODO: should be sorted | |
| `shouldBe` ([mkMetadata bytes 1024 | bytes <- sorted_encoded_data]) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| annotate | ||
| "Metadata stream roundtrip successful" | ||
| $ (decoded_metadata) | ||
| `shouldBe` ([mkMetadata bytes 1024 | bytes <- encoded_data]) -- TODO: should be sorted |
There was a problem hiding this comment.
The comparison uses encoded_data instead of sorted_encoded_data, which will cause test failures when the original data order differs from sorted order.
| `shouldBe` ([mkMetadata bytes 1024 | bytes <- encoded_data]) -- TODO: should be sorted | |
| `shouldBe` ([mkMetadata bytes 1024 | bytes <- sorted_encoded_data]) -- TODO: should be sorted |
| deriving (Show, Eq) | ||
|
|
||
| data Metadata = Metadata | ||
| { entries :: BS.ByteString -- TODO: reintroduce [MetadataEntry] ? |
There was a problem hiding this comment.
I'd reintroduce 'MetadataEntry' here, because otherwise it will likely be not very readable by the other clients.
| -- deriving (Show) | ||
|
|
||
| data MetadataFooter = MetadataFooter | ||
| { totalEntries :: Word64 |
There was a problem hiding this comment.
I think we need to add strictness here, just to be sure
qnikst
added this to the
Milestone 2 : Developpement of Haskell serialization code milestone
|
Given the changes upstream, I'm extracting some of the changes of this PR to their own PR in order to easily integrate these upstream changes as well as improve the review process. |
|
Closed in favor of #102 |
This is an initial implementation for the metadata record.
It's not finalized, but I realize it could benefit from a checkpoint review.